feat: rewrite with clean api #451
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
seuros
force-pushed
the
dummy
branch
5 times, most recently
from
1f818da to
398d9d8
Compare
Major changes: - Remove global monkey patching from ActiveRecord connections - Use model-level with_advisory_lock instead of connection-level - Add multi-database support (PostgreSQL, MySQL, SQLite) - SQLite models don't support advisory locks (returns false) - Add DatabaseCleaner configuration for multi-database tests - Rename SqliteTag to MemoryTag to avoid SQLite reserved prefix - Fix MySQL connection to use 127.0.0.1 instead of localhost - Use CRC32 for advisory lock names to fit MySQL 64-char limit - Add RuboCop and apply auto-corrections
There was a problem hiding this comment.
Pull Request Overview
This PR migrates closure_tree to use a dummy Rails app structure for testing and implements proper multi-database support, removing global monkey patching in favor of the new API patterns from the with_advisory_lock gem. The main changes include creating a standard Rails test environment with database-specific model classes and updating test infrastructure to support concurrent testing across PostgreSQL, MySQL, and SQLite.
Key changes:
- Migrated from standalone test setup to Rails dummy app structure with proper database configuration
- Implemented multi-database support with separate base classes for PostgreSQL, MySQL, and SQLite
- Converted Minitest describe/it syntax to Rails test methods using dynamic method definitions
Reviewed Changes
Copilot reviewed 95 out of 100 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/test_helper.rb | Complete rewrite to use Rails dummy app and configure multi-database testing |
| test/support/tag_examples.rb | Converted from describe/it to Rails test methods with dynamic definitions |
| test/dummy/ | New Rails dummy application structure with multi-database models and configuration |
| test/closure_tree/ | Updated test files to use new dummy app structure and converted syntax |
Comments suppressed due to low confidence (5)
test/support/tag_examples.rb:70
- The test method name has inconsistent spacing. It should be 'test_should_find_tag_by_valid_path' to match the naming pattern of other test methods.
define_method 'test_should find tag by valid path' do
test/support/tag_examples.rb:76
- The test method name has inconsistent spacing. It should be 'test_adds_children_through_add_child' to match the naming pattern of other test methods.
define_method 'test_adds children through add_child' do
test/support/tag_examples.rb:89
- The test method name has inconsistent spacing. It should be 'test_adds_children_through_collection' to match the naming pattern of other test methods.
define_method 'test_adds children through collection' do
test/support/tag_examples.rb:112
- The test method name contains spaces and special characters. It should be 'test_3_tag_collection_create_hierarchy' to follow standard Ruby method naming conventions.
define_method 'test_3 tag collection.create hierarchy' do
test/support/tag_examples.rb:192
- The test method name contains spaces. It should be 'test_3_tag_explicit_create_hierarchy' to follow standard Ruby method naming conventions.
define_method 'test_3 tag explicit_create hierarchy' do
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR migrates closure_tree to match the new API patterns from the with_advisory_lock gem, removing global monkey patching and implementing proper multi-database support.
Major Changes
1. Remove Global Monkey Patching
ActiveSupport.on_loadfor specific adapters only2. Multi-Database Support
3. Advisory Lock Implementation
model_class.with_advisory_lockinstead ofconnection.with_advisory_lock4. Test Infrastructure Updates
5. Code Quality
Testing
All tests pass with the new multi-database setup:
Breaking Changes
This removes the global monkey patching behavior. Applications relying on
connection.with_advisory_lockwill need to update to usemodel.with_advisory_lockinstead.